Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[flake8-tidy-imports] Allow banning global symbols (TID251) #15736

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[flake8-tidy-imports] Allow banning global symbols (TID251) #15736

wants to merge 3 commits into from

Conversation

InSyncWithFoo
Copy link
Contributor

Summary

Resolves #10079.

This change makes the following settings work:

[tool.ruff.lint.flake8-tidy-imports.banned-api]
"list".msg = "Lists considered harmful"
"builtins.tuple".msg = "Mutability for the win"

Before:

a = list()   # No error
b = tuple()  # No error

from builtins import tuple  # Error, but who imports from `builtins` anyway?
c = tuple()  # No error

After:

a = list()   # TID251: `list` is banned: Lists considered harmful
b = tuple()  # TID251: `builtins.tuple` is banned: Mutability for the win

from builtins import tuple  # TID251: `builtins.tuple` is banned: Mutability for the win
c = tuple()  # TID251: `builtins.tuple` is banned: Mutability for the win

Test Plan

cargo nextest run and cargo insta test.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 25, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+2 -0 violations, +0 -0 fixes in 1 projects; 54 projects unchanged)

scikit-build/scikit-build-core (+2 -0 violations, +0 -0 fixes) 

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ src/scikit_build_core/file_api/_cattrs_converter.py:66:17: TID251 `typing.Callable` is banned: Use collections.abc.Callable instead.
+ src/scikit_build_core/file_api/reply.py:116:17: TID251 `typing.Callable` is banned: Use collections.abc.Callable instead.

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
TID251 2 2 0 0 0

@InSyncWithFoo
Copy link
Contributor Author

This change seems to be for the worse, as both the imports and the usages are now reported:

@scikit-build/scikit-build-core:

from typing import Any, Callable, Dict, Type, TypeVar  # noqa: TID251
...
rich_print: Callable[[object], None]

Perhaps non-import references should only be reported if they are for global symbols?

@dylwil3 dylwil3 added the configuration Related to settings and configuration label Jan 30, 2025
iFreilicht
iFreilicht reviewed Mar 6, 2025
View reviewed changes
Copy link

@iFreilicht iFreilicht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand correctly: To fully ban a built-in API, I have to add both the list and builtins.list cases to the configuration, correct? That's the one thing I would like to see improved, I think a single line should ban all usage of the API. Personally I would say builtins.list is clearer. That case is also easy to check for in the code, which would allow to fix the problem @InSyncWithFoo brought up.

Generally, I'm in favor of this change. In my case it would be particularly useful because of round, because that has undesirable properties and want to make sure other developers use my custom replacement in my codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iFreilicht iFreilicht iFreilicht left review comments

Assignees
No one assigned
Labels
configuration Related to settings and configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow banning of certain builtins in "banned-api"
3 participants
@InSyncWithFoo @iFreilicht @dylwil3